Add Size hint enum - #2294

Clanker review

The core fix looks sound: the new SizeHint semantics repair the old buggy isize hint from #2086, the previously-ignored quickchecks are re-enabled and pass, and all workspace tests pass under both default and with_buffer_pool/--all-features. Versioning is fine too (codec-sv2 7.0.0, framing-sv2 8.0.0, parsers-sv2 0.6.0 are already unpublished major bumps past v1.11.1).

That said, a few things are worth addressing before merge — especially while these majors are still unpublished.

🔴 Should fix

1. The fix is one-sided: WithNoise still crashes on the same over-fill scenario — sv2/codec-sv2/src/decoder.rs:144, :168

WithNoise::next_frame keeps unguarded usize subtractions (*msg_len - self.noise_buffer.as_ref().len() at 144, header.encrypted_len() - IsBuffer::len(&self.noise_buffer) at 168) for exactly the buffer over-fill case this PR now handles gracefully in WithoutNoise via SizeHint::Surplus. Reproduced end-to-end with a real Noise session: calling writable() twice before next_frame (the same misuse the new WithoutNoise test exercises) panics with 'attempt to subtract with overflow' in debug, and in release wraps to ~usize::MAX, gets stored in missing_noise_b, and the next writable() requests an ~18-exabyte allocation (OOM/abort). WithoutNoise gets a clean recoverable error; WithNoise crashes.

2. size_hint's .expect() is a latent remote panic on the decode path — sv2/framing-sv2/src/framing.rs:192

Header::from_bytes(bytes).expect("header parsing only fails on short input") hard-codes an invariant of another module on the attacker-facing decode path. It's provably unreachable today (with len >= Header::SIZE the only fallible step is a U24 conversion of a u32 whose high byte is forced 0), but nothing enforces or tests that invariant, and header.rs carries fix: comments suggesting future validation. The day Header::from_bytes gains any validation, the first malformed 6-byte header a remote peer sends panics inside WithoutNoise::next_frame — a one-packet DoS for every non-noise role. Matching the Result and mapping Err to SizeHint::Missing keeps the code total (and removes the duplicated bytes.len() < Header::SIZE check, which Header::from_bytes re-does at header.rs:57).

3. The Surplus arm discards a complete, valid frame it could return — sv2/codec-sv2/src/decoder.rs:388

At Surplus(n) the buffer by construction holds exactly one complete, parseable frame followed by n bytes — the comment "the frame boundary is lost" is factually wrong (the header gives the boundary precisely; the drain is what loses it). let _ = self.buffer.get_data_owned() throws away both the frame and the surplus (which is the start of the next stream data), so a caller that treats non-MissingBytes errors as retryable silently loses the in-flight frame, keeps reading, and then parses mid-stream payload bytes as a header (e.g. [0,0,255,255,255,0] → msg_length 0xFFFFFF → ~16MB allocation, garbage frames, no integrity check). Consider consuming exactly one frame and retaining the surplus tail — or making the error unambiguously fatal.

🟠 API design (cheap to fix now, breaking later)

4. SizeHint name-collides with the pre-existing binary_sv2::SizeHint trait — sv2/codec-sv2/src/lib.rs:55

binary_sv2 already exports an unrelated SizeHint trait (sv2/binary-sv2/src/lib.rs:74), and codec-sv2 re-exports the new enum at crate root even though nothing in codec's own API produces or consumes it. Downstream use binary_sv2::*; use codec_sv2::*; (a routine pairing) hits E0659 "SizeHint is ambiguous". A distinct name (e.g. FrameSizeHint), or at minimum dropping the unused codec-sv2 re-export, avoids the trap.

5. missing_bytes() returns 0 for Surplus, and neither helper has a caller — sv2/framing-sv2/src/framing.rs:45-56

is_exact() and missing_bytes() have zero callers anywhere in the repo (every consumer pattern-matches the enum directly), and missing_bytes() == 0 for Surplus collapses exactly the Exact-vs-Surplus distinction the enum exists to preserve. The natural downstream idiom while size_hint(buf).missing_bytes() > 0 { read_more() } then from_bytes livelocks forever on a surplus buffer. Suggest dropping the helpers until a caller exists, or returning Option<usize> (None for Surplus).

6. Result<Self, SizeHint> makes Err(SizeHint::Exact) representable — sv2/framing-sv2/src/framing.rs:163

"Failed because the input is exactly one complete frame" is an impossible state every error consumer must still write an unreachable arm for, and a future refactor accidentally returning Err(SizeHint::Exact) type-checks silently. A dedicated two-variant error would be precise.

7. HandShakeFrame::from_bytes is now a byte-identical alias of from_bytes_unchecked — sv2/framing-sv2/src/framing.rs:278

The checked/unchecked naming pair implies a validation distinction that no longer exists (cf. Sv2Frame in the same file, where it's meaningful). Production code calls _unchecked (decoder.rs:233,236); the only from_bytes caller is one prop test. This PR already breaks from_bytes' signature, so collapsing the pair now costs no extra semver break — deferring it requires a second breaking release.

8. TlvError::FrameConstructionFailed is dead code that got migrated instead of removed — sv2/parsers-sv2/src/tlv/error.rs:25

Nothing in the repo constructs this variant except its own Display test, yet its payload was retyped (isize → SizeHint), freezing framing-sv2's SizeHint into a second crate's public error API with zero producers. Either wire it up where TLV frames are built (tlv/list.rs:91 only mentions the flow in a doc comment) or delete the variant.

🟡 Docs / error messages

9. UnexpectedTrailingBytes doc and Display describe a buffer that was already drained — sv2/codec-sv2/src/error.rs:58, :100

"The decoder buffer holds a complete frame plus…" / "Buffer holds {u} bytes beyond the end of the frame" are present tense, but next_frame drains the buffer before returning the error. A caller reading this reasonably retries next_frame to retrieve the still-"held" frame and gets MissingBytes(6) from an empty buffer. The docs should say the data was buffered and has been discarded.

10. The recovery contract is contradictory between doc and test — sv2/codec-sv2/src/decoder.rs:366 vs :610

The next_frame doc says "treat the stream as desynchronized", but the new test is named test_decoder_excess_bytes_drains_and_recovers and asserts the decoder "recovers" — which only works there because the test re-feeds a complete frame from byte 0, something a real desynchronized socket never provides. Suggest the doc state both halves explicitly: decoder state is reset and reusable, but the transport framing is lost, so resynchronize/reconnect.

11. MissingBytes Display says "Noise bytes" on the non-noise path — sv2/codec-sv2/src/error.rs:82

The rewritten docs make MissingBytes the primary error of the non-noise WithoutNoise decoder, but its Display still reads "Missing {u} Noise bytes" — an operator on a plain StandardDecoder build would misdiagnose a routine short read as an encryption-layer failure.

12. next_frame doc overstates what MissingBytes(n) promises — sv2/codec-sv2/src/decoder.rs:362

It says the count is "the number of bytes still required to complete the frame", omitting the caveat (documented on size_hint itself) that with fewer than Header::SIZE bytes buffered the count only covers completing the header. Code written to the documented contract misbehaves on every frame's first read.

🧹 Cleanup / tests

13. The SizeHint Display impl is dead and ungrammatical — sv2/framing-sv2/src/framing.rs:59

The only place a SizeHint is rendered (TlvError's Display, tlv/error.rs:38) uses {:?}, and its test pins the Debug form Missing(1) — so the obvious cleanup (switching to {}) breaks that assertion. Also "missing 1 bytes" / "1 surplus bytes". Either use the Display impl in TlvError now or drop it.

14. Duplicated reset-and-drain in the Surplus arm — sv2/codec-sv2/src/decoder.rs:391

The Surplus arm repeats the Exact arm's self.missing_b = Header::SIZE; + get_data_owned() bookkeeping, so the "ready for a fresh header" invariant lives in two match arms. A small shared reset-and-drain helper removes the divergence risk.

15. Minor points on the new tests — sv2/framing-sv2/src/framing.rs:663, sv2/codec-sv2/src/decoder.rs:624

• The replacement props narrow the domain (surplus capped at 64, payload < 4096 vs the old ±32767 / full-U24 range), and the zero-payload edge (Exact at exactly 6 bytes, Surplus on an empty-payload frame) is hit only probabilistically. Two cheap deterministic asserts would pin it: size_hint(&[0,0,1,0,0,0]) == Exact and size_hint(&[0,0,1,0,0,0,9,9,9]) == Surplus(3).
• In the decoder test, the decoder.writable().len() call at line 624 creates the surplus as a side effect (get_writable pre-extends by missing_b), so surplus is tautologically missing_b. A comment noting that mechanism (or an explicit oversized copy) would make the intent robust.

Also: sv2/framing-sv2/src/framing.rs:163 — from_bytes keeps mut bytes: B / bytes.as_mut() although size_hint takes &[u8] and the bound already includes AsRef<[u8]>; bytes.as_ref() with a non-mut binding is equivalent and doesn't advertise mutation that never happens.

🤖 Reviewed with Claude Code

@GitGab19 I didn't took all the suggestion but few of them. We will standardize few things with codec/framing refactor.